-
Notifications
You must be signed in to change notification settings - Fork 15.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[7392] [cpp] Remove dead code path for map key of type enum in JSON parsing #16567
[7392] [cpp] Remove dead code path for map key of type enum in JSON parsing #16567
Conversation
@sbenzaquen friendly ping -- please advise if we should continue with the bugfix or remove the dead code block. |
@jskeet, I am reaching out to you since you worked with me on #7392 and this PR is first in line to fix the remaining corner case for C++. Can you help me reach @sbenzaquen -- I suspect they are not seeing github notifications here. Btw, nothing urgent here, I am not blocked on this -- just minimizng the amount of things I have open. |
Have sent a ping by internal email. |
Sorry for the delay. |
@sbenzaquen -- thank you! Updated the PR accordingly. |
@sbenzaquen thank you for the review! There was (I believe) an unrelated test failure that blocked merge: I merged with latest |
…arsing (#16567) # Changes Remove dead code path -- we don't allow enums to be map keys ([proto2 spec](https://protobuf.dev/programming-guides/proto2/#maps), [proto3 spec](https://protobuf.dev/programming-guides/proto3/#maps)). In other words the case block `case FieldDescriptor::TYPE_ENUM` is dead code. Potential enum type keys will be caught in `default: return lex.Invalid("unsupported map key type");` block below similar to other unsupported map key types like double. # Motivation While working on fixing `IgnoreUnknownEnumStringValueInMap` conformance tests for cpp ([related issue](#7392)) I stumbled upon a bug where we pass the wrong `field` parameter to the enum parsing function. In this scope: * the variable `field` is a map field of the message that holds the map. This field is not of enum type, it's a repeated message of map entires. * the variable `key_field` is the key field of the map message entry. This field is the enum type that we need to parse here. The function is long, so I clarified it here: ```cpp template <typename Traits> absl::Status ParseMap(JsonLexer& lex, Field<Traits> field, Msg<Traits>& msg) { (..) return lex.VisitObject( [&](LocationWith<MaybeOwnedString>& key) -> absl::Status { (..) return Traits::NewMsg( field, msg, [&](const Desc<Traits>& type, Msg<Traits>& entry) -> absl::Status { auto key_field = Traits::KeyField(type); switch (Traits::FieldType(key_field)) { (..) case FieldDescriptor::TYPE_ENUM: { MaybeOwnedString key_str = key.value; auto e = ParseEnumFromStr<Traits>(lex, key_str, /** bug here **/ field); ``` The correct reference should be `key_field`. Instead of fixing the bug and leaving the dead code, it's better to remove the dead block alltogether. Closes #16567 COPYBARA_INTEGRATE_REVIEW=#16567 from noom:anton--7392--fix-map-key-nit d992b8a FUTURE_COPYBARA_INTEGRATE_REVIEW=#16567 from noom:anton--7392--fix-map-key-nit d992b8a PiperOrigin-RevId: 634509516
…arsing (protocolbuffers#16567) # Changes Remove dead code path -- we don't allow enums to be map keys ([proto2 spec](https://protobuf.dev/programming-guides/proto2/#maps), [proto3 spec](https://protobuf.dev/programming-guides/proto3/#maps)). In other words the case block `case FieldDescriptor::TYPE_ENUM` is dead code. Potential enum type keys will be caught in `default: return lex.Invalid("unsupported map key type");` block below similar to other unsupported map key types like double. # Motivation While working on fixing `IgnoreUnknownEnumStringValueInMap` conformance tests for cpp ([related issue](protocolbuffers#7392)) I stumbled upon a bug where we pass the wrong `field` parameter to the enum parsing function. In this scope: * the variable `field` is a map field of the message that holds the map. This field is not of enum type, it's a repeated message of map entires. * the variable `key_field` is the key field of the map message entry. This field is the enum type that we need to parse here. The function is long, so I clarified it here: ```cpp template <typename Traits> absl::Status ParseMap(JsonLexer& lex, Field<Traits> field, Msg<Traits>& msg) { (..) return lex.VisitObject( [&](LocationWith<MaybeOwnedString>& key) -> absl::Status { (..) return Traits::NewMsg( field, msg, [&](const Desc<Traits>& type, Msg<Traits>& entry) -> absl::Status { auto key_field = Traits::KeyField(type); switch (Traits::FieldType(key_field)) { (..) case FieldDescriptor::TYPE_ENUM: { MaybeOwnedString key_str = key.value; auto e = ParseEnumFromStr<Traits>(lex, key_str, /** bug here **/ field); ``` The correct reference should be `key_field`. Instead of fixing the bug and leaving the dead code, it's better to remove the dead block alltogether. Closes protocolbuffers#16567 COPYBARA_INTEGRATE_REVIEW=protocolbuffers#16567 from noom:anton--7392--fix-map-key-nit d992b8a PiperOrigin-RevId: 634516984
Changes
Remove dead code path -- we don't allow enums to be map keys (proto2 spec, proto3 spec). In other words the case block
case FieldDescriptor::TYPE_ENUM
is dead code. Potential enum type keys will be caught indefault: return lex.Invalid("unsupported map key type");
block below similar to other unsupported map key types like double.Motivation
While working on fixing
IgnoreUnknownEnumStringValueInMap
conformance tests for cpp (related issue) I stumbled upon a bug where we pass the wrongfield
parameter to the enum parsing function.In this scope:
field
is a map field of the message that holds the map. This field is not of enum type, it's a repeated message of map entires.key_field
is the key field of the map message entry. This field is the enum type that we need to parse here.The function is long, so I clarified it here:
The correct reference should be
key_field
.Instead of fixing the bug and leaving the dead code, it's better to remove the dead block alltogether.